Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: implement transaction context #315

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

federicobond
Copy link
Member

This is a draft implementation of transaction context propagation via contextvars.

@federicobond federicobond force-pushed the feat/transaction-context branch 2 times, most recently from 7f2da6b to b622d21 Compare April 10, 2024 04:23
@federicobond federicobond linked an issue Apr 10, 2024 that may be closed by this pull request
@federicobond federicobond changed the title [DRAFT] feat: implement transaction context feat: implement transaction context Apr 10, 2024
]

_evaluation_context = EvaluationContext()

_hooks: typing.List[Hook] = []

_transaction_context_propagator: TransactionContextPropagator = (
NoopTransactionContextPropagator()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the SDK initialize a functional transaction context propagator by default?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do something similar to this in JS. What would the alternative be?

Copy link
Member Author

@federicobond federicobond May 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a default implementation that is not a no-op. But it could introduce a performance penalty if you are not using it, so probably better to default to no-op. We might want to put out a warning if you call set_transaction_context with a NoopTransactionContextPropagator though.



def get_transaction_context() -> EvaluationContext:
return _transaction_context_propagator.get_transaction_context()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the right API or do we want something based on decorators/context managers?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate on this? The main use case for accessing transaction context would be within the SDK when it merges context prior to flag evaluation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the comment should have been in the set_transaction_context. I was wondering if we should do something like this instead:

with start_transaction_context(context):
    continue_transaction()  # context will be cleaned up after the block completes

Similar to the way it works in JS by taking a callback.

Copy link

codecov bot commented Apr 12, 2024

Codecov Report

Attention: Patch coverage is 93.47826% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 97.18%. Comparing base (44f1223) to head (c31a2fa).

Files Patch % Lines
openfeature/api.py 75.00% 2 Missing ⚠️
openfeature/transaction_context.py 94.73% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #315      +/-   ##
==========================================
- Coverage   97.32%   97.18%   -0.15%     
==========================================
  Files          26       28       +2     
  Lines        1198     1244      +46     
==========================================
+ Hits         1166     1209      +43     
- Misses         32       35       +3     
Flag Coverage Δ
unittests 97.18% <93.47%> (-0.15%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Implement transaction context
2 participants